Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invoke Function Without Parameters #604

Merged
merged 7 commits into from
Aug 15, 2018
Merged

Invoke Function Without Parameters #604

merged 7 commits into from
Aug 15, 2018

Conversation

akkaash
Copy link
Contributor

@akkaash akkaash commented Aug 3, 2018

*Issue #556 *

Invoke Function Without Parameters

Adds support to sam local invoke to accept no event argument.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall. Just some small feedback

event_data = "{}"
elif no_args and event != STDIN_FILE_NAME:
# Do not know what the user wants. no_args and event both passed in.
raise UserException("no_args and event both given. Do not know what to do")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this to some thing of the effect: "no_args and event cannot be used together. Please provide only one."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

help="JSON file containing event data passed to the Lambda function during invoke. If this option "
"is not specified, we will default to reading JSON from stdin")
@click.option("--no-args", is_flag=True, default=False, help="Invoke Function without parameters")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, I don't think --no-args describes what this does. This is really giving a default value to the event. Maybe something like --no-event (so it matches with the rest of the command) and then update the help text to be Invoke the Function with an empty event dictionary?

@sooddhruv Do you have any thoughts here?

Copy link
Contributor

@dhruvsood dhruvsood Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about not requiring the --no-args/--no-event option? sam local invoke "FunctionName" could just invoke the function with a default payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sooddhruv sam local invoke "FunctionName" currently allows the user to use STDIN to provide the event payload. This would cause a conflict with current behavior. What do you think?
@jfuss I agree. I like --no-event better for the option name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akkaash That makes sense. I can see an argument for changing this behavior but something maybe we shouldn't do now. Let's just leave this as --no-event and then we can see if there is a path/reason to change this behavior to what @sooddhruv suggested.

@@ -94,3 +94,14 @@ def test_invoke_when_function_writes_stderr(self):
process_stderr = b"".join(process.stderr.readlines()).strip()

self.assertIn("Docker Lambda is writing to stderr", process_stderr.decode('utf-8'))

def test_invoke_raises_exception_with_noargs_and_event(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a small integ test for providing --no-agrs and making sure the function invoked and returns as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@jfuss jfuss added area/lambda-invoke stage/in-progress A fix is being worked on labels Aug 8, 2018
@jfuss
Copy link
Contributor

jfuss commented Aug 10, 2018

@akkaash Can you rebase and handle the conflicts? The PR with the addition of --region is conflicting here. Should be a straight forward thing to resolve.

@akkaash
Copy link
Contributor Author

akkaash commented Aug 13, 2018

@jfuss I've updated the pr. Multiple merge conflicts had to be fixed.

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akkaash Two small comments on the changes and then I think this will be ready to merge. 🎉

region=self.region)

msg = str(ex_ctx.exception)
print(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this print statement from the tests? Assuming you where using this during testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops. My bad. I've fixed that.

# Pass all inputs to setup necessary context to invoke function locally.
# Handler exception raised by the processor for invalid args and print errors
try:
if no_event and event == STDIN_FILE_NAME:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this out of the try except block like it was before. Having a tight try except is a good practice to follow.

What do you think about update the checks to something like this? I think it is a little easier to reason and read (less mental checks). This is optional as I am ok with what you have as well. Just trying to see if we could simplify these checks in some manner.

if no_event and event != STDIN_FILE_NAME:
  raise UserException()

if no_event:
  event_data = "{}"
else:
  event_data = _get_event(event)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how this is less cognitive overload for a reader. Made the change.

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks for going through the changes here!

@akkaash
Copy link
Contributor Author

akkaash commented Aug 14, 2018

@jfuss is there anything else I need to do in order to merge this in? 😃

@jfuss
Copy link
Contributor

jfuss commented Aug 15, 2018

@akkaash Nothing is needed. I had to update the branch and never got back to it after the build passed.

@jfuss jfuss merged commit 799c390 into aws:develop Aug 15, 2018
@jfuss jfuss added stage/accepted Accepted and will be fixed and removed stage/in-progress A fix is being worked on labels Aug 15, 2018
jfuss added a commit that referenced this pull request Aug 28, 2018
* fix: Functional tests must run on localhost to work in Windows (#552)

* fix: spacing typo in Log statement in start-lambda (#559)

* docs: Fix syntax highlighting in README.md (#561)

* docs: Change jest to mocha in Nodejs init README (#564)

* docs: Fix @mhart link in README (#562)

* docs(README): removed cloudtrail, added SNS to generate-event (#569)

* docs: Update repo name references (#577)

* feat(debugging): Fixing issues around debugging Golang functions. (#565)

* fix(init): Improve current init samples around docs and fixes (#558)

* docs(README): Update launch config to SAM CLI from SAM Local (#587)

* docs(README): Update sample code for calling Local Lambda Invoke (#584)

* refactor(init): renamed handler for camel case, moved callback call up (#586)

* chore: aws-lambda-java-core 1.1.0 -> 1.2.0 for java sam init (#578)

* feat(validate): Add profile and region options (#582)

Currently, `sam validate` requires AWS Creds (due to the SAM Translator).
This commits adds the ability to pass in the credientials through a profile
that is configured through `aws configure`.

* docs(README): Update README prerequisites to include awscli (#596)

* fix(start-lambda): Remove Content-Type Header check (#594)

* docs: Disambiguation "Amazon Kinesis" (#599)

* docs: Adding instructions for how to add pyenv to your PATH for Windows (#600)

* docs: Update README with small grammar fix (#601)

* fix: Update link in NodeJS package.json (#603)

* docs: Creating instructions for Windows users to install sam (#605)

* docs: Adding a note directing Windows users to use pipenv (#606)

* fix: Fix stringifying λ environment variables when using Python2 (#579)

* feat(generate-event): Added support for 50+ events (#612)

* feat(invoke): Add region parameter to all invoke related commands (#608)

* docs: Breaking up README into separate files to make it easier to read (#607)

* chore: Update JVM size params to match docker-lambda (#615)

* feat(invoke): Invoke Function Without Parameters through --no-event (#604)

* docs: Update advanced_usage.rst with clarification on --env-vars usage (#610)

* docs: Remove an extra word in the sam packaging command (#618)

* UX: Improves event names to reflect Lambda Event Sources (#619)

* docs: Fix git clone typo in installation docs (#630)

* docs(README): Callout go1.x runtime support (#631)

* docs(installation): Update sam --version command (#634)

* chore(0.6.0): SAM CLI Version bump (#635)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lambda-invoke stage/accepted Accepted and will be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants